Enhance Audit Log Data Beyond Privacy Requests #3331

SteveDMurphy · 2023-05-18T22:33:28Z

Closes fde#8

Code Changes

Create a new model and migration to house the data
Add middleware to write the non-GET data to the server
Create a new middleware.py to house functionality
Set a config variable to allow the logging to be enabled or disabled (disabled by default)

Steps to Confirm

To enable the new audit logging, add FIDES__SECURITY__ENABLE_AUDIT_LOG_RESOURCE_MIDDLEWARE: "True" to the compose file then running nox -s dev
Create a system (or do anything the requires a POST or PUT
Validate the record is written to the audit log resource table (select * from public.audit_log_resource;)
Click around and do other things to ensure that a record is properly written
Cypress tests pass for all routes when middleware enabled

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

Description Of Changes

This PR will address the who and when with a high-level what more than the what (detail) has changed. A design doc will follow as part of this issue closure.

Interestingly, the new middleware severely impacted pytest (i.e. it didn't run). Adding the config variable to disallow it was an option I was already interested in but became a necessity for that reason.

cypress · 2023-05-18T22:53:14Z

Passing run #2215 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `7d05a60` into `6cd8cbf`...
Project: fides	Commit: `7e1a821aab ℹ️`
Status: Passed	Duration: 00:53 💡
Started: May 26, 2023 6:36 PM	Ended: May 26, 2023 6:37 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

@seanpreston

@seanpreston I think this is the same implementation that is used elsewhere but doesn't seem to yield the same result. I have thrown breakpoints on it and watched it through a few different ways. One caveat is to see it working elsewhere, you also need to create a different user and sign in as that user.

src/fides/api/ctl/sql_models.py

…8-audit-log-resource

codecov · 2023-05-25T14:26:19Z

Codecov Report

Patch coverage: 59.72% and project coverage change: -0.12 ⚠️

Comparison is base (6cd8cbf) 87.24% compared to head (7d05a60) 87.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3331      +/-   ##
==========================================
- Coverage   87.24%   87.12%   -0.12%     
==========================================
  Files         311      312       +1     
  Lines       18587    18659      +72     
  Branches     2368     2377       +9     
==========================================
+ Hits        16216    16257      +41     
- Misses       1955     1981      +26     
- Partials      416      421       +5

Impacted Files	Coverage Δ
src/fides/api/main.py	`77.55% <50.00%> (-3.14%)`	⬇️
src/fides/api/middleware.py	`55.55% <55.55%> (ø)`
src/fides/api/ctl/sql_models.py	`97.72% <100.00%> (+0.06%)`	⬆️
src/fides/core/config/security_settings.py	`98.88% <100.00%> (+0.01%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

SteveDMurphy · 2023-05-25T15:02:03Z

@seanpreston still adding some more tests but wanted to at least get this back on the radar so it can go with the release - thanks again for the help!!

…8-audit-log-resource

.fides/db_dataset.yml

adamsachs

overall i think this looks like a really solid first iteration @SteveDMurphy. the code is pretty easy to follow, i like how you've organized things and you've set up a great foundation! gating this behind a config var (that defaults to false) i think is a great and necessary choice to get this out in the wild while mitigating any risk.

i've got some minor cleanup comments, some of which would be nice to address before merging, i think. beyond that, there are a couple of more "meaty" questions about how we can ensure this all runs properly in the background and stays decoupled from actual API/application tasks - as i note in the comments, i don't think these need to block this initial iteration, but i believe they should be seriously considered before we're ready to deploy this in a production (or any client) environment.

let me know what you think and if you'd like to link up at all to discuss!

src/fides/api/ctl/sql_models.py

src/fides/api/middleware.py

src/fides/api/main.py

src/fides/core/config/security_settings.py

src/fides/api/middleware.py

src/fides/api/ctl/sql_models.py

src/fides/api/middleware.py

src/fides/api/oauth/utils.py

Co-authored-by: Adam Sachs <[email protected]>

tests/ops/api/v1/test_middleware.py

adamsachs · 2023-05-26T18:19:37Z

merging in main then merging the PR - CI had passed previously, just wanted to make sure the fides_db_scan still passed after my db_dataset.yml tweak!

adamsachs

looks great as a first iteration, follow ups have been noted and issues will be created 👍

Co-authored-by: Adam Sachs <[email protected]>

SteveDMurphy added 2 commits May 18, 2023 23:24

table model and migration for audit_log_resource

5782aac

initial middleware to capture resource data

1c237fe

seanpreston reviewed May 24, 2023

View reviewed changes

src/fides/api/ctl/sql_models.py Show resolved Hide resolved

SteveDMurphy added 5 commits May 25, 2023 01:49

split out functionality to new module for testing

4c7bf59

Merge branch 'main' of github.com:ethyca/fides into SteveDMurphy-fde-…

1a2815f

…8-audit-log-resource

tidy up pylint and remaining mypy

395b8f1

fix migration conflicts, update db_dataset.yml

e157748

add config option for the middleware, defaults off

8a677e7

SteveDMurphy requested a review from seanpreston May 25, 2023 15:01

SteveDMurphy added 4 commits May 25, 2023 16:37

first half of testing

f8a92e0

additional tests for extracting keys

e978ffd

Merge branch 'main' of github.com:ethyca/fides into SteveDMurphy-fde-…

e24cf47

…8-audit-log-resource

changelog

85ce77c

SteveDMurphy self-assigned this May 25, 2023

SteveDMurphy marked this pull request as ready for review May 25, 2023 20:24

SteveDMurphy requested a review from adamsachs May 26, 2023 12:59

Merge branch 'main' into SteveDMurphy-fde-8-audit-log-resource

84a600b

adamsachs reviewed May 26, 2023

View reviewed changes

.fides/db_dataset.yml Outdated Show resolved Hide resolved

adamsachs reviewed May 26, 2023

View reviewed changes

SteveDMurphy and others added 3 commits May 26, 2023 11:44

Make IGNORED_AUDIT_LOG_RESOURCE_PATHS a set instead of a list

3187fed

Co-authored-by: Adam Sachs <[email protected]>

Actually use get_client_user_id!

ae3fbe0

Co-authored-by: Adam Sachs <[email protected]>

fix up comments, remove redundant function

3994582

adamsachs reviewed May 26, 2023

View reviewed changes

tests/ops/api/v1/test_middleware.py Show resolved Hide resolved

undo indent of db_dataset.yml

0a2f66a

Merge branch 'main' into SteveDMurphy-fde-8-audit-log-resource

7d05a60

adamsachs approved these changes May 26, 2023

View reviewed changes

adamsachs merged commit b86e77a into main May 26, 2023

adamsachs deleted the SteveDMurphy-fde-8-audit-log-resource branch May 26, 2023 18:56

adamsachs added a commit that referenced this pull request May 26, 2023

Enhance Audit Log Data Beyond Privacy Requests (#3331)

c2b3177

Co-authored-by: Adam Sachs <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Audit Log Data Beyond Privacy Requests #3331

Enhance Audit Log Data Beyond Privacy Requests #3331

SteveDMurphy commented May 18, 2023 •

edited

Loading

cypress bot commented May 18, 2023 •

edited

Loading

codecov bot commented May 25, 2023 •

edited

Loading

SteveDMurphy commented May 25, 2023

adamsachs left a comment

adamsachs commented May 26, 2023

adamsachs left a comment

Enhance Audit Log Data Beyond Privacy Requests #3331

Enhance Audit Log Data Beyond Privacy Requests #3331

Conversation

SteveDMurphy commented May 18, 2023 • edited Loading

Code Changes

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

cypress bot commented May 18, 2023 • edited Loading

Passing run #2215 ↗︎

codecov bot commented May 25, 2023 • edited Loading

Codecov Report

SteveDMurphy commented May 25, 2023

adamsachs left a comment

Choose a reason for hiding this comment

adamsachs commented May 26, 2023

adamsachs left a comment

Choose a reason for hiding this comment

SteveDMurphy commented May 18, 2023 •

edited

Loading

cypress bot commented May 18, 2023 •

edited

Loading

codecov bot commented May 25, 2023 •

edited

Loading